Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(nodebuilder/core): non default values from config.toml are written over by flag defaults in specific circumstances #3526

Merged
merged 5 commits into from
Jun 27, 2024

Conversation

ramin
Copy link
Collaborator

@ramin ramin commented Jun 25, 2024

identified by @tac0turtle (TYBG) whereby, in the specific case where you had edited you node store's config values to be non-default for Core.RPCPort or Core.GRPCPort, if you were to then provide a different value for Core.IP via the --core.ip flag at node start time, then the flag defaults for core.rpc.port and core.grpc.port would revert the values to defaults and override the custom values in config.toml. Fix is to check that the default value had been reset in the flag, and only use the default if no value had been set in the passed in config, which in most cases is already loaded as default or the config.toml values.

Took liberty to add some tests, and update some duplication of default values while in here.

fixes #3523

…lues from config.toml would be overwritten by defaults if any core.ip was provided as a cli flag
@ramin ramin added the kind:fix Attached to bug-fixing PRs label Jun 25, 2024
…refer fail on validation, also fill out tests for Validate
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@renaynay renaynay enabled auto-merge (squash) June 27, 2024 10:59
@renaynay renaynay merged commit 1fa5aa6 into main Jun 27, 2024
29 of 30 checks passed
@renaynay renaynay deleted the fix/ramin/core-flag-parsing branch June 27, 2024 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:fix Attached to bug-fixing PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nodebuilder: Config changes not seen by node.Start
3 participants